Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

de-opacify the logic behind how we choose to delete users from a hub db #5078

Merged
merged 10 commits into from
Sep 28, 2023
Merged

de-opacify the logic behind how we choose to delete users from a hub db #5078

merged 10 commits into from
Sep 28, 2023

Conversation

shaneknapp
Copy link
Contributor

@shaneknapp shaneknapp commented Sep 27, 2023 •
edited
Loading

this script was a little confusing to parse, so i added a type check, additional output and cleaned up a confusing if statement.

@shaneknapp shaneknapp changed the title de-opaqueify the logic behind how we choose to delete users from a hub db de-opacify the logic behind how we choose to delete users from a hub db Sep 27, 2023
@shaneknapp
Copy link
Contributor Author

shaneknapp commented Sep 27, 2023 •
edited
Loading

some sample output of a run (without deleting users, of course):

user: XXXX
last login: 2023-09-01 19:52:01.354394+00:00
24hrs since last login: False
server: None
Deleting XXXX

user: XXXX
last login: 2023-09-27 20:22:54.683580+00:00
24hrs since last login: True
server: /user/XXXX
Not deleting XXXX

user: XXXX
last login: 2023-09-27 17:30:56.352292+00:00
24hrs since last login: True
server: None
Not deleting XXXX

user: XXXX
last login: 2023-08-07 21:55:12.489883+00:00
24hrs since last login: False
server: None
Deleting XXXX

user: XXXX
last login: 2023-09-21 18:15:22.552542+00:00
24hrs since last login: False
server: None
Deleting XXXX

user: XXXX
last login: 2023-09-24 05:07:04.235891+00:00
24hrs since last login: False
server: None
Deleting XXXX

user: XXXX
last login: 2023-08-11 04:23:35.881508+00:00
24hrs since last login: False
server: None
Deleting XXXX

....

164 of 169: deleting XXXX
Skipped due to dry run.
165 of 169: deleting XXXX
Skipped due to dry run.
166 of 169: deleting XXXX
Skipped due to dry run.
167 of 169: deleting XXXX
Skipped due to dry run.
168 of 169: deleting XXXX
Skipped due to dry run.
169 of 169: deleting XXXX
Skipped due to dry run.

ryanlovett
ryanlovett requested changes Sep 27, 2023
View reviewed changes
Copy link
Collaborator

@ryanlovett ryanlovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Described a potential issue.

scripts/delete-unused-users.py Outdated Show resolved Hide resolved
@shaneknapp
Copy link
Contributor Author

shaneknapp commented Sep 27, 2023 •
edited
Loading

some testing output from the class check with last_activity = 'asdfasdf':

For user XXXX, expected datetime.datetime class for last_activity but got <class 'str'> instead.
Traceback (most recent call last):
  File "/home/sknapp/src/datahub/scripts/./delete-unused-users.py", line 71, in <module>
    asyncio.run(main())
  File "/home/sknapp/miniconda3/envs/dh/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/home/sknapp/miniconda3/envs/dh/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/home/sknapp/src/datahub/scripts/./delete-unused-users.py", line 50, in main
    raise
RuntimeError: No active exception to reraise

and just to be sure, last_activity = asyncio:

For user XXXX, expected datetime.datetime class for last_activity but got <class 'module'> instead.
Traceback (most recent call last):
  File "/home/sknapp/src/datahub/scripts/./delete-unused-users.py", line 71, in <module>
    asyncio.run(main())
  File "/home/sknapp/miniconda3/envs/dh/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/home/sknapp/miniconda3/envs/dh/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/home/sknapp/src/datahub/scripts/./delete-unused-users.py", line 50, in main
    raise
RuntimeError: No active exception to reraise

ryanlovett
ryanlovett requested changes Sep 27, 2023
View reviewed changes
Copy link
Collaborator

@ryanlovett ryanlovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can omit the last_activity = asyncio line? (was that for debugging?)

Also the isinstance check now means you can omit the check for last_activity in the new line 55. So it'd just be if was_active_last_day or user['server'] is not None:.

scripts/delete-unused-users.py Outdated Show resolved Hide resolved
@shaneknapp shaneknapp merged commit 8f30cf0 into berkeley-dsep-infra:staging Sep 28, 2023
@shaneknapp shaneknapp deleted the minor-refactor branch September 28, 2023 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanlovett ryanlovett ryanlovett requested changes

Assignees
No one assigned
Labels
python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shaneknapp @ryanlovett